refactor/node api design changes #459

domharrington · 2022-05-26T14:32:26Z

🧰 Changes

High level list of changes

removed the expressMiddleware export, switched to export a single readmeio.log function
deprecated the existing readmeio.log function (which was primarily used for calling from plain node.js http servers)
removed the getProjectBaseUrl function
added a new POST method integration test, which exposed some issues in almost all of the current sdks/implementation examples

So now the API looks like this:

app.use((req, res, next) => {
  const logId = readmeio.log(process.env.README_API_KEY, req, res, {
    // User's API Key
    apiKey: 'owlbert-api-key',
    // Username to show in the dashboard
    label: 'Owlbert',
    // User's email address
    email: '[email protected]',
  });

  res.setHeader('x-documentation-url', `https://docs.example.com/logs/${logId}`);
  return next();
});

If you want us to set the x-documentation-url, you now have to call setHeader() yourself.

You can see the express sample app diff here: 150d913

Outstanding items:

docs
update hapi example app
update fastify example app
remove dependency on express http types
Should I remove baseLogUrl entirely in this release, or allow it to be passed in?

Open questions:

is there a better way to surface errors for people having issues? We can't just await readmeio.log() because of the queue, and the asynchronous nature of the request happening after the response has been sent.

🧬 QA & Testing

Do the tests pass? Does this look good? ✅

…iddleware

I think we'll have to tell people to call this manually now, since we can't perform an asynchronous lookup without asking them to `await` the call to the function, which will look kinda bad and like we're blocking.

Remove existing readmeio.log() implementation, this should be possible to be done via this default function now

@ilias-t

For some reason this meant that in commonjs you had to do the following 🤮: ```js const readmeio = require('readmeio').default ``` The way I've done it now, it works in ts-node as I expect, but for some reason I have to do this in the tests: ```js import * as readmeio from '../src'; ``` I have no idea why 🤷‍♂️ @ilias-t any ideas here?

… first

domharrington · 2022-05-26T15:25:05Z

Another possible solution to the getProjectBaseUrl() thing is to do something like this:

import readmeio from 'readmeio';

readmeio.config({ apiKey: process.env.README_API_KEY });

app.use((req, res, next) => {
  readmeio.log(req, res, {
    // User's API Key
    apiKey: 'owlbert-api-key',
    // Username to show in the dashboard
    label: 'Owlbert',
    // User's email address
    email: '[email protected]',
  });

  return next();
});

Then we can call it from inside the .config() function and hope🤞that it's there by the time we encounter the first log.

Another one here... have the user do it explicitly themselves and return the logId from the function:

app.use(async (req, res, next) => {
  const logId = readmeio.log(process.env.README_API_KEY, req, res, {
    // User's API Key
    apiKey: 'owlbert-api-key',
    // Username to show in the dashboard
    label: 'Owlbert',
    // User's email address
    email: '[email protected]',
  });

  res.setHeader('x-documentation-url', `https://docs.example.com/logs/${logId}`);

  return next();
});

I actually don't hate this. It's not as if we're relying on any kind of standard here, so it shouldnt matter where they put it. This is definitely the easiest/most flexible.

Dashron · 2022-05-26T16:48:02Z

for the header, my preferences are...

res.setHeader('x-documentation-url', https://docs.example.com/logs/${logId}`);` so there are zero network requests and the user does the work, once, ever
const baseLogUrl = await readmeio.getProjectBaseUrl(apiKey); but it would be great if it could be more explicit that it was cached, or have it not cache and put some code in the example that caches it. maybe wrap the middleware to memo baseLogUrl.

as for the overall pattern, I love it. the callback was always a little clunky, and moving auth data between middleware seems pretty janky. This way they can slap the log into the same middleware as their auth parsing and just reuse the variables in the grouping params.

will review code shortly.

packages/node/__tests__/lib/construct-payload.test.ts

packages/node/__tests__/lib/process-request.test.ts

packages/node/package.json

packages/node/src/lib/construct-payload.ts

Dashron · 2022-05-26T17:02:26Z

packages/node/src/lib/log.ts

+
+  // Make the log call
+  metricsAPICall(readmeApiKey, json).catch(e => {
+    // Silently discard errors and timeouts.


this always worries me. There should be some way for a user to get errors even if not in development mode, maybe add an option and log it as console.error?

Yeah agreed. Tbh I really dislike the idea of a development mode, it's messy and just hides data currently in the dash (it needs many improvements). Can you think of any other API ideas to log errors when they happen?

Maybe we could accept a logger option?

readmeio.log(process.env.README_API_KEY, req, res, group, { logger: console });

Or we could use https://www.npmjs.com/package/debug?

I like the logger option and also that debug package is great. We should really be enforcing something like debug in all our codebases cause atm there isn't a dominant convention.

Yeahh the tough thing is DEBUG is node.js specific (afaik?) so we have to either figure out a similar pattern that can be applied to all languages OR find the dominant pattern per language and apply that.

One idea that @erunion had at one point would be for us to attempt to figure out what env they're running in, and log stuff out if they're in development. In Node.js we could use NODE_ENV, in .net you can use app.Environment.IsDevelopment() etc etc.

Dashron · 2022-05-26T17:08:41Z

packages/node/src/lib/log.ts

+// It's unfortunate that this isn't accessible
+// natively. This may take up lots of memory on
+// big responses, we can make it configurable in future
+function patchResponse(res) {


wish we could find something better for this :/

Yeah I've tried, I really have. res is a ServerResponse which is a WriteableStream. It can have stuff piped to it, but there's no way to pipe that somewhere else or listen to it. Would love for you to see if there's a better way around this!

packages/node/src/lib/process-request.ts

packages/node/__tests__/lib/get-project-base-url.test.ts

packages/node/src/lib/construct-payload.ts

packages/node/src/lib/log.ts

ilias-t · 2022-05-26T20:03:03Z

packages/node/src/lib/log.ts

+  // Ensures the buffer length is between 1 and 30
+  const bufferLength = clamp(options.bufferLength || config.bufferLength, 1, 30);
+
+  const baseLogUrl = options.baseLogUrl || undefined;


nit: I generally prefer using null instead of undefined when explicitly setting a value, something like const { baseLogUrl = null } = options;

Yeah fair point, tbh I'm not sure what to do about this option overall. Idk whether it makes sense for us to be doing this for them or encouraging it via code sample instead: #562

ilias-t · 2022-05-26T20:03:17Z

packages/node/src/lib/log.ts

+      // {
+      //   ...req,
+
+      //   // Shallow copying `req` destroys `req.headers` on Node 16 so we're re-adding it.


packages/node/src/lib/log.ts

domharrington · 2022-05-27T08:58:14Z

Fixed up the hapi example app here: 17edfc0 much nicer IMHO, much more consistent with the express one.

Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once #459 is merged in.

Make the hapi/express examples more consistent with each other

This is dependent on #557 being merged because currently the POST data implementation is broken in the Python SDK

Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once #459 is merged in.

packages/node/examples/fastify/index.js

packages/node/examples/hapi/index.js

This can be reverted when #560 is merged in

packages/node/examples/fastify/index.js

ilias-t · 2022-08-26T17:46:03Z

packages/node/src/lib/log.ts

+
+  // Make the log call
+  metricsAPICall(readmeApiKey, json).catch(e => {
+    // Silently discard errors and timeouts.


I like the logger option and also that debug package is great. We should really be enforcing something like debug in all our codebases cause atm there isn't a dominant convention.

ilias-t · 2022-08-26T17:48:53Z

packages/node/src/lib/log.ts

+  patchResponse(res);
+
+  // @todo we should remove this and put this in the code samples
+  if (baseLogUrl !== undefined && typeof baseLogUrl === 'string') {


nit: wouldn't if (typeof baseLogUrl === 'string') be the same given it will filter out all undefined values?

erunion

Kind of a bummer that we're losing the one liner Express integration but overall I like this flow much better as it allows us to standardize our integration docs for everyone.

.github/MAINTAINERS.md

packages/node/__tests__/index.test.ts

Co-authored-by: Jon Ursenbach <[email protected]>

# Conflicts: # __tests__/integration-metrics.test.js

@ilias-t

* refactor(node): first pass at changing API design away from express middleware * refactor(node): add support for nested express urls via req.originalUrl * refactor(node): remove unneeded test to do with grouping function * refactor(node): fix up bufferLength tests * refactor(node): move get-project-base-url tests elsewhere I think we'll have to tell people to call this manually now, since we can't perform an asynchronous lookup without asking them to `await` the call to the function, which will look kinda bad and like we're blocking. * refactor(node): get `res._body` tests working * refactor(node): get `req.body` tests working * refactor(node): tidy up tests * refactor(node): add some tests for the validation errors * refactor(node): tidy up * refactor(node): fix up other tests * refactor(node): rename default export to readmeio.log() Remove existing readmeio.log() implementation, this should be possible to be done via this default function now * refactor(node): tidy up * refactor(node): switch from using default export For some reason this meant that in commonjs you had to do the following 🤮: ```js const readmeio = require('readmeio').default ``` The way I've done it now, it works in ts-node as I expect, but for some reason I have to do this in the tests: ```js import * as readmeio from '../src'; ``` I have no idea why 🤷‍♂️ @ilias-t any ideas here? * refactor(integration): updating express app to use new method signature * refactor(node): add test showing usage of calling getProjectBaseUrl() first * refactor(integration): remove unused lint rule * refactor(integration): update hapi code sample to use new api * chore: lint * test: add test for har `pageref` * chore: lint * feat: return with the logId from readme.log() function * chore: lint after `main` branch merge * chore: fix some tests due to missing dependency * refactor: remove `getProjectBaseUrl()` function * refactor: removing express type dependency from logging functions * refactor: remove seemingly unused code We already have a test for this which passes with this code commented out: https://github.com/readmeio/metrics-sdks/blob/e0576f460d66f9ab23cfd2bc22b8f65c5dd01cb1/packages/node/__tests__/index.test.ts#L210-L247 I think this is being handled here: https://github.com/readmeio/metrics-sdks/blob/e0576f460d66f9ab23cfd2bc22b8f65c5dd01cb1/packages/node/src/lib/process-request.ts#L181-L184 * feat(integration): add POST method test Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once readmeio/metrics-sdks#459 is merged in. * fix(integration): update hapi test with new api design * fix(integration): get fastify demo working with new api design Make the hapi/express examples more consistent with each other * chore: code comment about raw.setHeaders piece * fix(integration): forgot to re-export verifyWebhook after refactor * feat(integration): add support for POST methods to php and c# * feat(integration): add support for POST methods to python/flask This is dependent on readmeio/metrics-sdks#557 being merged because currently the POST data implementation is broken in the Python SDK * chore(python): lint * chore: temp fix php integration test This can be reverted when readmeio/metrics-sdks#560 is merged in * refactor(node): switch to using spread operator instead of arr.slice() * Update .github/MAINTAINERS.md Co-authored-by: Jon Ursenbach <[email protected]> * chore: updated comment to be clearer Co-authored-by: Jon Ursenbach <[email protected]>

* feat(integration): add POST method test Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once #459 is merged in. * fix(php): update postData to push into text instead of params[] * fix: rewriting how we retrieve data out of the request * feat: adding a heap of new unit tests to the metrics integration suite * fix: the node SDK not capturing text/plain and +json payloads * fix: node sdk not capturing `application/x-www-url-formencoded` payloads * fix: optionally skipping multipart tests for sdks that dont support it * fix: stop setting postData if there's none to set * fix: adding missing env vars * fix(python): adding query strings to urls, dont add postData if empty * fix(python): handling of `x-www-form-urlencoded` requests * fix: don't crash in node or php if a json payload is corrupted * fix: python code standards issues * fix: replaying test improvemnts i made * fix: moving the webhook test over to using the node protocol * fix: removing dead code * fix: regenerating the package lockfile * fix: broken tests * fix: compatibility issues with fastify and hapi * feat: fleshing out some useful makefiles * feat: more makefiles * fix: no longer setting an empty `text` property on urlencoded requests * feat: makefile targets for the .net sdk * fix: problem where dotnet request.url didn't contain querystrings * fix: problem where .net would send nullish postData on get requests * fix: broken php tests * fix: downgrading the python ci tests in docker to use node 16 * ci: docker python debugging * fix: remove uneeded python docker workarounds for node 18 * fix(integration): exit early from tests if child process didn't start * bug: attempt to fix python ci (#565) * Revert "fix: remove uneeded python docker workarounds for node 18" This reverts commit 0c9df96. * Revert "ci: docker python debugging" This reverts commit 067f95f. * Revert "fix: downgrading the python ci tests in docker to use node 16" This reverts commit af5c110. * fix(python/integration): bump timeout of HTTPConnectionPool I think we were hitting this timeout sometimes from within the container This makes it pass locally at least, lets see if this works on gh * fix(integration): disable undici on flask integration tests * fix: try to use mocha instead of jest * fix: add commented out experimental fetch disabling * fix: getting tests in mocha working * fix: re-disabling undici in node 18 * ci: attempts to get python working in docker always * ci: disabling python flask integration in ci for now Co-authored-by: Jon Ursenbach <[email protected]> Co-authored-by: Dom Harrington <[email protected]> Co-authored-by: Dom Harrington <[email protected]>

* feat(integration): add POST method test Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once readmeio/metrics-sdks#459 is merged in. * fix(php): update postData to push into text instead of params[] * fix: rewriting how we retrieve data out of the request * feat: adding a heap of new unit tests to the metrics integration suite * fix: the node SDK not capturing text/plain and +json payloads * fix: node sdk not capturing `application/x-www-url-formencoded` payloads * fix: optionally skipping multipart tests for sdks that dont support it * fix: stop setting postData if there's none to set * fix: adding missing env vars * fix(python): adding query strings to urls, dont add postData if empty * fix(python): handling of `x-www-form-urlencoded` requests * fix: don't crash in node or php if a json payload is corrupted * fix: python code standards issues * fix: replaying test improvemnts i made * fix: moving the webhook test over to using the node protocol * fix: removing dead code * fix: regenerating the package lockfile * fix: broken tests * fix: compatibility issues with fastify and hapi * feat: fleshing out some useful makefiles * feat: more makefiles * fix: no longer setting an empty `text` property on urlencoded requests * feat: makefile targets for the .net sdk * fix: problem where dotnet request.url didn't contain querystrings * fix: problem where .net would send nullish postData on get requests * fix: broken php tests * fix: downgrading the python ci tests in docker to use node 16 * ci: docker python debugging * fix: remove uneeded python docker workarounds for node 18 * fix(integration): exit early from tests if child process didn't start * bug: attempt to fix python ci (#565) * Revert "fix: remove uneeded python docker workarounds for node 18" This reverts commit 0c9df96c34a8d5724b391478d009b2c056e097e6. * Revert "ci: docker python debugging" This reverts commit 067f95f9f52529aada953440fb346fbd38da4d1a. * Revert "fix: downgrading the python ci tests in docker to use node 16" This reverts commit af5c110c6354d9fd7d0c27d77fcda5ca92cf539f. * fix(python/integration): bump timeout of HTTPConnectionPool I think we were hitting this timeout sometimes from within the container This makes it pass locally at least, lets see if this works on gh * fix(integration): disable undici on flask integration tests * fix: try to use mocha instead of jest * fix: add commented out experimental fetch disabling * fix: getting tests in mocha working * fix: re-disabling undici in node 18 * ci: attempts to get python working in docker always * ci: disabling python flask integration in ci for now Co-authored-by: Jon Ursenbach <[email protected]> Co-authored-by: Dom Harrington <[email protected]> Co-authored-by: Dom Harrington <[email protected]>

* refactor: improved integration test suite (#560) * feat(integration): add POST method test Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once #459 is merged in. * fix(php): update postData to push into text instead of params[] * fix: rewriting how we retrieve data out of the request * feat: adding a heap of new unit tests to the metrics integration suite * fix: the node SDK not capturing text/plain and +json payloads * fix: node sdk not capturing `application/x-www-url-formencoded` payloads * fix: optionally skipping multipart tests for sdks that dont support it * fix: stop setting postData if there's none to set * fix: adding missing env vars * fix(python): adding query strings to urls, dont add postData if empty * fix(python): handling of `x-www-form-urlencoded` requests * fix: don't crash in node or php if a json payload is corrupted * fix: python code standards issues * fix: replaying test improvemnts i made * fix: moving the webhook test over to using the node protocol * fix: removing dead code * fix: regenerating the package lockfile * fix: broken tests * fix: compatibility issues with fastify and hapi * feat: fleshing out some useful makefiles * feat: more makefiles * fix: no longer setting an empty `text` property on urlencoded requests * feat: makefile targets for the .net sdk * fix: problem where dotnet request.url didn't contain querystrings * fix: problem where .net would send nullish postData on get requests * fix: broken php tests * fix: downgrading the python ci tests in docker to use node 16 * ci: docker python debugging * fix: remove uneeded python docker workarounds for node 18 * fix(integration): exit early from tests if child process didn't start * bug: attempt to fix python ci (#565) * Revert "fix: remove uneeded python docker workarounds for node 18" This reverts commit 0c9df96. * Revert "ci: docker python debugging" This reverts commit 067f95f. * Revert "fix: downgrading the python ci tests in docker to use node 16" This reverts commit af5c110. * fix(python/integration): bump timeout of HTTPConnectionPool I think we were hitting this timeout sometimes from within the container This makes it pass locally at least, lets see if this works on gh * fix(integration): disable undici on flask integration tests * fix: try to use mocha instead of jest * fix: add commented out experimental fetch disabling * fix: getting tests in mocha working * fix: re-disabling undici in node 18 * ci: attempts to get python working in docker always * ci: disabling python flask integration in ci for now Co-authored-by: Jon Ursenbach <[email protected]> Co-authored-by: Dom Harrington <[email protected]> Co-authored-by: Dom Harrington <[email protected]> * feat: getting the django tests all running locally * chore: remove debug * fix: code standards issues * chore: alphabetizing the docker-compose file Co-authored-by: Dom Harrington <[email protected]> Co-authored-by: Dom Harrington <[email protected]>

* chore: move flask README.md into flask/ folder * feat(python): add basic skeleton for test django app * feat(python): working e2e django setup Still needs wiring up to integration test server and the app needs thinning out * feat(python): ready the django app for integration test - check for existence of README_API_KEY and exit(1) if not present - set default port from os.getenv("PORT") * feat(python): working e2e integration test Had to make the following changes to get it to pass fully: - Fix date format in django.py. Same fix as here: #480 - Reduce default BUFFER_LENGTH down to 1. This mirrors what's happening in the other SDKs for now, but would be good to increase this in future. * feat(python): wire up django integration test to gh actions * fix(python): bug with POST data in har generation We are setting `rm_content_length` on the request object here: - [django](https://github.com/readmeio/metrics-sdks/blob/4217fefe9417ef39130913dc51989ab021e0405c/packages/python/readme_metrics/django.py#L23) - [flask](https://github.com/readmeio/metrics-sdks/blob/4217fefe9417ef39130913dc51989ab021e0405c/packages/python/readme_metrics/flask_readme.py#L34) But in the PayloadBuilder we were trying to fetch `content_length` without the rm_ prefix. This meant that POST bodies weren't getting picked up. * feat(integration/python/django): add support for POST method test * chore(integration/python/django): lint * fix(integration/python/django): test with start date Fixed after df7243d#diff-8f208c3f785655a644003c8703cba81313de50ce7aef3068a6b5dd858fcf2577R20 * chore(python): ignore virtualenv folders from linter * chore(python): lint * fix(python): ensure django integration test is running in gh actions * fix(python/integration): bump timeout of HTTPConnectionPool I think we were hitting this timeout sometimes from within the container This makes it pass locally at least, lets see if this works on gh * chore: updating the django branch with HEAD (#566) * refactor: improved integration test suite (#560) * feat(integration): add POST method test Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once #459 is merged in. * fix(php): update postData to push into text instead of params[] * fix: rewriting how we retrieve data out of the request * feat: adding a heap of new unit tests to the metrics integration suite * fix: the node SDK not capturing text/plain and +json payloads * fix: node sdk not capturing `application/x-www-url-formencoded` payloads * fix: optionally skipping multipart tests for sdks that dont support it * fix: stop setting postData if there's none to set * fix: adding missing env vars * fix(python): adding query strings to urls, dont add postData if empty * fix(python): handling of `x-www-form-urlencoded` requests * fix: don't crash in node or php if a json payload is corrupted * fix: python code standards issues * fix: replaying test improvemnts i made * fix: moving the webhook test over to using the node protocol * fix: removing dead code * fix: regenerating the package lockfile * fix: broken tests * fix: compatibility issues with fastify and hapi * feat: fleshing out some useful makefiles * feat: more makefiles * fix: no longer setting an empty `text` property on urlencoded requests * feat: makefile targets for the .net sdk * fix: problem where dotnet request.url didn't contain querystrings * fix: problem where .net would send nullish postData on get requests * fix: broken php tests * fix: downgrading the python ci tests in docker to use node 16 * ci: docker python debugging * fix: remove uneeded python docker workarounds for node 18 * fix(integration): exit early from tests if child process didn't start * bug: attempt to fix python ci (#565) * Revert "fix: remove uneeded python docker workarounds for node 18" This reverts commit 0c9df96. * Revert "ci: docker python debugging" This reverts commit 067f95f. * Revert "fix: downgrading the python ci tests in docker to use node 16" This reverts commit af5c110. * fix(python/integration): bump timeout of HTTPConnectionPool I think we were hitting this timeout sometimes from within the container This makes it pass locally at least, lets see if this works on gh * fix(integration): disable undici on flask integration tests * fix: try to use mocha instead of jest * fix: add commented out experimental fetch disabling * fix: getting tests in mocha working * fix: re-disabling undici in node 18 * ci: attempts to get python working in docker always * ci: disabling python flask integration in ci for now Co-authored-by: Jon Ursenbach <[email protected]> Co-authored-by: Dom Harrington <[email protected]> Co-authored-by: Dom Harrington <[email protected]> * feat: getting the django tests all running locally * chore: remove debug * fix: code standards issues * chore: alphabetizing the docker-compose file Co-authored-by: Dom Harrington <[email protected]> Co-authored-by: Dom Harrington <[email protected]> * chore(integration): remove res.on('end') test finishers I dont think it's actually required and everything seems to pass without * chore(integration): add test watch command for mocha with nodemon * chore(integration): turn off django in CI * refactor(python/integration): only ask for README_API_KEY when running server * fix(python/django): move instantiation of grouping_function (#567) * fix(python/django): move instantiation of grouping_function Previously we were trying to import the grouping function in MetricsApiConfig which in Django's case gets called from settings.py (before the models have been setup). So if your grouping function was located in a file that happened to use a model (or was a function on a model) it would error with the following: ``` django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet. ``` This PR moves the loading of the grouping function (if it needs to be imported) into Metrics instead of in the Config, this is late enough for django to have started so you can use models. * test(python/integration): add a very lightweight model to example app This makes the top level integration test fail with the issue: ``` django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet. ``` Which helps us to ensure it can't happen again * chore(python/integration): lint Co-authored-by: Jon Ursenbach <[email protected]>

domharrington added 17 commits May 25, 2022 12:57

refactor(node): first pass at changing API design away from express m…

5b064c0

…iddleware

refactor(node): add support for nested express urls via req.originalUrl

8365fd3

refactor(node): remove unneeded test to do with grouping function

ec1c08a

refactor(node): fix up bufferLength tests

15b026a

refactor(node): move get-project-base-url tests elsewhere

3a7b181

I think we'll have to tell people to call this manually now, since we can't perform an asynchronous lookup without asking them to `await` the call to the function, which will look kinda bad and like we're blocking.

refactor(node): get res._body tests working

9b097bd

refactor(node): get req.body tests working

edd32b5

refactor(node): tidy up tests

e9a318d

refactor(node): add some tests for the validation errors

425b94f

refactor(node): tidy up

ad8b7d8

refactor(node): fix up other tests

da6d627

refactor(node): rename default export to readmeio.log()

da9db38

Remove existing readmeio.log() implementation, this should be possible to be done via this default function now

refactor(node): tidy up

4003342

refactor(integration): updating express app to use new method signature

150d913

refactor(node): add test showing usage of calling getProjectBaseUrl()…

f34aa1c

… first

refactor(integration): remove unused lint rule

7098501

domharrington requested review from ilias-t, gratcliff and mjcuva May 26, 2022 15:06

Dashron reviewed May 26, 2022

View reviewed changes

ilias-t reviewed May 26, 2022

View reviewed changes

refactor(integration): update hapi code sample to use new api

17edfc0

domharrington added 2 commits May 27, 2022 10:00

chore: lint

0f3477d

test: add test for har pageref

14a2c4c

ilias-t requested review from mjcuva and removed request for mjcuva June 1, 2022 22:18

domharrington added 3 commits August 23, 2022 15:57

feat(integration): add POST method test

0567582

Right now the hapi/fastify examples are not working properly for accepting an incoming POST body. This adds a test to expose that flaw, but does not yet fix it. I think this refactor makes the most sense to finish once #459 is merged in.

fix(integration): update hapi test with new api design

552961a

fix(integration): get fastify demo working with new api design

d4e0ade

Make the hapi/express examples more consistent with each other

domharrington marked this pull request as ready for review August 24, 2022 13:11

domharrington added 7 commits August 24, 2022 14:13

chore: code comment about raw.setHeaders piece

edc168a

fix(integration): forgot to re-export verifyWebhook after refactor

2ac995e

Merge branch 'main' into refactor/node-api-design-changes

5444d7a

feat(integration): add support for POST methods to php and c#

b7f8011

feat(integration): add support for POST methods to python/flask

04cde20

This is dependent on #557 being merged because currently the POST data implementation is broken in the Python SDK

Merge branch 'main' into refactor/node-api-design-changes

1936d0b

chore(python): lint

e0e2651

domharrington requested a review from erunion August 25, 2022 13:28

domharrington commented Aug 25, 2022

View reviewed changes

packages/node/examples/fastify/index.js Show resolved Hide resolved

domharrington commented Aug 25, 2022

View reviewed changes

packages/node/examples/hapi/index.js Show resolved Hide resolved

chore: temp fix php integration test

97592bf

This can be reverted when #560 is merged in

ilias-t approved these changes Aug 26, 2022

View reviewed changes

domharrington mentioned this pull request Aug 29, 2022

Make Node.js code examples as simple as possible #561

Open

2 tasks

refactor(node): switch to using spread operator instead of arr.slice()

c6cd973

erunion approved these changes Aug 29, 2022

View reviewed changes

.github/MAINTAINERS.md Outdated Show resolved Hide resolved

packages/node/__tests__/index.test.ts Show resolved Hide resolved

domharrington and others added 3 commits August 30, 2022 15:12

Update .github/MAINTAINERS.md

7ab4e87

Co-authored-by: Jon Ursenbach <[email protected]>

Merge branch 'main' into refactor/node-api-design-changes

633be34

# Conflicts: # __tests__/integration-metrics.test.js

chore: updated comment to be clearer

c311e01

domharrington merged commit 2d04da3 into main Aug 30, 2022

domharrington deleted the refactor/node-api-design-changes branch August 30, 2022 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor/node api design changes #459

refactor/node api design changes #459

domharrington commented May 26, 2022 •

edited

Loading

domharrington commented May 26, 2022 •

edited

Loading

Dashron commented May 26, 2022

Dashron May 26, 2022

domharrington May 27, 2022 •

edited

Loading

ilias-t Aug 26, 2022

domharrington Aug 29, 2022

domharrington Aug 29, 2022

Dashron May 26, 2022

domharrington May 27, 2022

ilias-t May 26, 2022

domharrington Aug 29, 2022

ilias-t May 26, 2022

domharrington commented May 27, 2022

ilias-t Aug 26, 2022

ilias-t Aug 26, 2022

erunion left a comment

refactor/node api design changes #459

refactor/node api design changes #459

Conversation

domharrington commented May 26, 2022 • edited Loading

🧰 Changes

🧬 QA & Testing

domharrington commented May 26, 2022 • edited Loading

Dashron commented May 26, 2022

Choose a reason for hiding this comment

domharrington May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domharrington commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erunion left a comment

Choose a reason for hiding this comment

domharrington commented May 26, 2022 •

edited

Loading

domharrington commented May 26, 2022 •

edited

Loading

domharrington May 27, 2022 •

edited

Loading